vplay-12110 simplified #875
Conversation
Reason for change: To create common POC app for direct Rialto integration Risks: Low Priority: P1 Signed-off-by: ryadav698 <emailofrajatyadav@gmail.com>
Reason for change: To create common POC app for direct Rialto integration Risks: Low Priority: P1 Signed-off-by: ryadav698 <emailofrajatyadav@gmail.com>
Reason for change: To create common POC app for direct Rialto integration Risks: Low Priority: P1 Signed-off-by: ryadav698 <emailofrajatyadav@gmail.com>
Reason for change: To create common POC app for direct Rialto integration Risks: Low Priority: P1 Signed-off-by: ryadav698 <emailofrajatyadav@gmail.com>
Reason for change: To create common POC app for direct Rialto integration Risks: Low Priority: P1 Signed-off-by: ryadav698 <emailofrajatyadav@gmail.com>
Reason for change: To create common POC app for direct Rialto integration Risks: Low Priority: P1 Signed-off-by: ryadav698 <emailofrajatyadav@gmail.com>
Signed-off-by: ryadav698 <emailofrajatyadav@gmail.com>
Signed-off-by: ryadav698 <emailofrajatyadav@gmail.com>
Signed-off-by: ryadav698 <emailofrajatyadav@gmail.com>
Signed-off-by: ryadav698 <emailofrajatyadav@gmail.com>
Signed-off-by: ryadav698 <emailofrajatyadav@gmail.com>
Signed-off-by: ryadav698 <emailofrajatyadav@gmail.com>
Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
…nto rialto-pipeline2.cpp; deprecating need for rialto-test2.cpp and rialto-pipeline2.h Signed-off-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces new Rialto proof-of-concept (POC) test applications to the AAMP codebase, specifically adding "rialtoPOC3" as a direct app-to-Rialto Server integration test. The purpose is to create a simplified test variant (rialto-test3.cpp) that works similarly to rialto-test2.cpp but with common interfaces and architecture.
Changes:
- Added new rialtoPOC3 test application with direct Rialto C++ API integration
- Added new rialtoPOC2 test application using app-managed GStreamer pipelines with Rialto sinks
- Modified mp4demux.hpp to expose internal structures for direct access by test code
- Updated root CMakeLists.txt to include the new test/rialtoPOC subdirectory
Reviewed changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| test/rialtoPOC/rialto-test3.cpp | Main test application using Rialto public API for media playback |
| test/rialtoPOC/rialto-pipeline3.h | Header defining GstMediaPipeline wrapper interface for Rialto integration |
| test/rialtoPOC/rialto-pipeline3.cpp | Implementation of Rialto pipeline wrapper with delegated method calls |
| test/rialtoPOC/rialto-pipeline2.cpp | Implementation of app-managed GStreamer pipeline with Rialto sinks |
| test/rialtoPOC/README.rtfd/TXT.rtf | Documentation describing the three test variants and their architectures |
| test/rialtoPOC/CMakeLists.txt | Build configuration for both rialtoPOC2 and rialtoPOC3 executables |
| test/gstTestHarness/mp4demux.hpp | Changed access specifier from private to public for internal structures |
| CMakeLists.txt | Added rialtoPOC subdirectory to build system |
test/rialtoPOC/rialto-test3.cpp
Outdated
| */ | ||
|
|
||
| #include <stdio.h> | ||
| #include <cassert> |
There was a problem hiding this comment.
Assert statements are prohibited in production code per the coding guidelines. While this is test code, consider using proper error handling with logging instead of asserts throughout this file.
test/rialtoPOC/rialto-test3.cpp
Outdated
| printf("loading rialtotest %s\n", fullpath); | ||
|
|
||
| FILE *f = fopen(fullpath, "rb"); | ||
| assert(f); |
There was a problem hiding this comment.
Replace assert with proper error handling. Use logging and return error status instead of relying on assertions which are disabled in release builds.
test/rialtoPOC/rialto-test3.cpp
Outdated
| unsigned char *ptr = (unsigned char *)malloc(len); | ||
| assert(ptr); |
There was a problem hiding this comment.
Using C-style malloc/free is discouraged in C++ code. Use std::vector<uint8_t> or std::unique_ptr<uint8_t[]> with new[] instead, which provides automatic memory management and exception safety.
| long len = ftell(f); | ||
| if (len > 0) | ||
| { | ||
| unsigned char *ptr = (unsigned char *)malloc(len); |
There was a problem hiding this comment.
Replace C-style cast with static_cast or use modern C++ memory management. This cast is unnecessary if using std::vector or smart pointers.
test/rialtoPOC/rialto-test3.cpp
Outdated
| streamFormat = StreamFormat::UNDEFINED; | ||
| break; | ||
| default: | ||
| assert(0); |
There was a problem hiding this comment.
Replace assert with proper error handling. Log an error message and return a failure status instead.
| m_sourceState[i] = {}; | ||
|
|
||
| GstElement *playbin = gst_element_factory_make("playbin", nullptr); | ||
| assert(playbin); |
There was a problem hiding this comment.
Replace assert with proper error handling. Use logging and handle the error gracefully.
| { | ||
| GstElement *videoSink = | ||
| gst_element_factory_make("rialtomsevideosink", "video-sink"); | ||
| assert(videoSink); |
There was a problem hiding this comment.
Replace assert with proper error handling. Use logging and handle the error gracefully.
| { | ||
| GstElement *audioSink = | ||
| gst_element_factory_make("rialtomseaudiosink", "audio-sink"); | ||
| assert(audioSink); |
There was a problem hiding this comment.
Replace assert with proper error handling. Use logging and handle the error gracefully.
| class Mp4Demux | ||
| { | ||
| private: | ||
| public: // temp workaround - used directly in rialtoTest |
There was a problem hiding this comment.
This 'temp workaround' comment indicates a design issue. Instead of exposing private members, consider adding proper accessor methods to maintain encapsulation. This violates the principle of information hiding.
| find_path( RIALTO_INCLUDE_DIR NAMES IMediaPipeline.h PATH_SUFFIXES rialto) | ||
| find_library( RIALTO_LIBRARY NAMES libRialtoClient.so RialtoClient ) | ||
| include( FindPackageHandleStandardArgs ) | ||
| find_package_handle_standard_args( RIALTO DEFAULT_MSG | ||
| RIALTO_LIBRARY RIALTO_INCLUDE_DIR ) |
There was a problem hiding this comment.
This Rialto discovery logic is duplicated for both rialtoPOC2 and rialtoPOC3 targets (lines 60-78 and 101-119). Extract this into a function or a separate CMake module to follow DRY principles.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@pstroffolino I've opened a new pull request, #879, to work on those changes. Once the pull request is ready, I'll request review from you. |
VPLAY-12110 simplifications for direct Rialto test app
Reason for Change: I want to make the rialto-test2.cpp variant work like rialto-test3.cpp, with common Rialto-test.cpp and (ideally) common rialto-pipeline.h - differences only in rialto-pipeline2.cpp and rialto-pipeline3.cpp
Risk: Medium